Skip to content

ensure there is no slicing risk on inherited classes#218

Closed
atsju wants to merge 1 commit intogithubdoe:masterfrom
atsju:JST/slicing
Closed

ensure there is no slicing risk on inherited classes#218
atsju wants to merge 1 commit intogithubdoe:masterfrom
atsju:JST/slicing

Conversation

@atsju
Copy link
Collaborator

@atsju atsju commented Aug 10, 2025

Clazy the tool that checks some Qt and C++ compliance actually has level2 checks that I did not activate because 2Y ago we had 10k warnings on level1

I looked into clazy-copyable-polymorphic because this is a real risk.

What I mainly did is

  • make boundary an abstract class. Constructor, copy and move being protected, it cannot be instantiated. Thus no slicing is possible. Never.
  • adding final and removing some wrong virtual is only to help Clazy identify there is no risk and remove the warning.

To conclude: I can now confirm there was no slicing happening. The changes remove the risk to have it happen someday.

I choose the path of minimal changes to not mess with the code and simplify review. If you think rectangleOutline and ellipseOutline will never be coded, we could simplify the code a lot. Remove inheritance and theses classes.
I like the KISS principle "Keep It Short and Simple". Less code means less maintenance and less problems.

Kindly let me know if you prefer this pull request code with minimal changes or a more complete rewrite to simplify the code.

@atsju atsju requested review from githubdoe and gr5 August 10, 2025 09:06
@gr5
Copy link
Collaborator

gr5 commented Aug 10, 2025

I had to learn what "slicing" meant:

Object slicing is a phenomenon in object-oriented programming, particularly in languages like C++, where an object of a derived class is assigned to a variable of its base class, resulting in the loss of the derived class's specific data and methods. Essentially, the "extra" parts of the derived class are "sliced off" during the assignment, leaving only the base class portion. This can lead to unexpected behavior and loss of functionality, as the derived class's unique attributes are no longer accessible.

Copy link
Collaborator

@gr5 gr5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes seem fine. I was able to build and run it just fine as well.

@githubdoe
Copy link
Owner

I don't remember much about those unused outline types. I might have thought they would be needed but then never did need them. If not used there is no plan to use them now by me so they can be removed as desired.

@atsju
Copy link
Collaborator Author

atsju commented Aug 10, 2025

Thank you both. I may do a second pull request and you can choose best option.

@atsju
Copy link
Collaborator Author

atsju commented Aug 13, 2025

I close this one. With latest information it seems what we want is #220

@atsju atsju closed this Aug 13, 2025
@atsju atsju deleted the JST/slicing branch August 16, 2025 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

Comments